chore: use testify's require in tests - #2792
Conversation
📊 API Diff Results
|
c97e92d to
10c74ca
Compare
10c74ca to
33eea1f
Compare
33eea1f to
a0a237e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new recorder-mode fail-fast can mask log-identity failures, and a couple of updated tests use require.Fail from goroutines via observeAll, which is unreliable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR standardizes the Grafana alertcheck test suite on testify/require for fail-fast assertions, adds the testify module dependency, and introduces a recorder-mode “fail fast” in internal/gate/check.go when from is before the recorded header’s StartedAt.
Changes:
- Replace hand-rolled
t.Fatal/t.Errorpatterns across tests withrequire.*assertions. - Add
github.com/stretchr/testify(and indirect YAML dep) togo.mod/go.sum. - Add a recorder-mode early exit in
check.gofor the statically-detectablefrom < StartedAtbound violation.
File summaries
| File | Description |
|---|---|
| grafana-alertcheck/internal/gate/watch_test.go | Mechanical migration of assertions to require. |
| grafana-alertcheck/internal/gate/watch_daemon_test.go | Mechanical migration of assertions to require in integration-style daemon tests. |
| grafana-alertcheck/internal/gate/source_test.go | Mechanical migration of assertions to require. |
| grafana-alertcheck/internal/gate/schedule_test.go | Mechanical migration of assertions to require. |
| grafana-alertcheck/internal/gate/resolve_test.go | Mechanical migration of assertions to require. |
| grafana-alertcheck/internal/gate/parse_state_test.go | Mechanical migration of assertions to require. |
| grafana-alertcheck/internal/gate/parse_ruler_test.go | Mechanical migration of assertions to require. |
| grafana-alertcheck/internal/gate/log_test.go | Mechanical migration of assertions to require. |
| grafana-alertcheck/internal/gate/jsonreq_test.go | Mechanical migration of assertions to require. |
| grafana-alertcheck/internal/gate/duration_test.go | Mechanical migration of assertions to require. |
| grafana-alertcheck/internal/gate/coverage_test.go | Mechanical migration of assertions to require. |
| grafana-alertcheck/internal/gate/classify_test.go | Mechanical migration of assertions to require. |
| grafana-alertcheck/internal/gate/check.go | Adds recorder-mode fail-fast when from precedes StartedAt. |
| grafana-alertcheck/internal/gate/check_test.go | Updates tests to require and adds coverage for the new fail-fast path. |
| grafana-alertcheck/go.mod | Adds testify dependency (and indirect YAML dep). |
| grafana-alertcheck/go.sum | Adds checksums for newly introduced dependencies. |
| grafana-alertcheck/cmd/grafana-alertcheck/watch_test.go | Mechanical migration of CLI tests to require. |
| grafana-alertcheck/cmd/grafana-alertcheck/table_test.go | Mechanical migration of table rendering tests to require. |
| grafana-alertcheck/cmd/grafana-alertcheck/main_test.go | Mechanical migration of CLI entrypoint tests to require. |
| grafana-alertcheck/cmd/grafana-alertcheck/list_test.go | Mechanical migration of CLI list tests to require. |
| grafana-alertcheck/cmd/grafana-alertcheck/check_test.go | Mechanical migration of CLI check tests to require. |
Review details
Suppressed comments (1)
grafana-alertcheck/internal/gate/check_test.go:895
- This responder can be invoked from goroutines via
observeAllduring the drain wait.require.FailtriggersFailNowfrom a worker goroutine, which doesn't reliably stop the test. Prefer returning an error (the caller path already makes the test fail by violating therequire.NoErrorassertion).
src := newCheckSource(func(title string, _ int) (Observation, error) {
require.Fail(t, fmt.Sprintf("the drain wait polled skipped rule %q", title))
return Observation{}, errors.New("unexpected poll")
})
- Files reviewed: 20/21 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4855fcd to
a87fc14
Compare
a87fc14 to
ca3ccc6
Compare
Migrates all test assertions from hand-rolled
t.Fatal/Errorto testifyrequire, adds thetestifydependency, and adds a recorder-mode fail-fast incheck.goforfrom < StartedAt.Review focus: the
check.gofail-fast (the only non-test change); the rest is a mechanical assertion swap.